Skip to content

[SPARK-59442][SQL] Zero the reserved payload when writing a null nanosecond timestamp to UnsafeRow - #58746

Closed
stevomitric wants to merge 2 commits into
apache:masterfrom
stevomitric:stevomitric/nanos-groupby-null-key
Closed

stevomitric wants to merge 2 commits into
apache:masterfrom
stevomitric:stevomitric/nanos-groupby-null-key

Conversation

@stevomitric

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

UnsafeWriter.write(int, TimestampNanosVal) stores a nanosecond timestamp as a 16-byte variable-length payload (modeled on CalendarInterval) and reserves that space even for a null value so the slot can be updated in place later. On the null branch it set the null bit but left the reserved 16 bytes untouched. The writer's buffer is reused across rows, so a null nanosecond value inherited whatever bytes the previously written row left in that slot.

Two logically-equal null rows could therefore produce different UnsafeRow byte contents, breaking the invariant that equal rows encode identically -- the invariant that UnsafeRow hashing and equality rely on. This is the root cause of a nullable nanosecond-timestamp GROUP BY / join key splitting its NULLs across multiple groups.

The fix zeroes the reserved payload on the null branch, mirroring the in-place update path (UnsafeRow.setTimestampNTZNanos / setTimestampLTZNanos), which already calls TimestampNanosRowValues.zeroPayload.

Why are the changes needed?

Correctness: a null nanosecond timestamp must encode canonically so that null grouping/join keys compare and hash identically. Without it, GROUP BY (and any key-based operator) over a nullable TIMESTAMP_NTZ(p) / TIMESTAMP_LTZ(p) column can silently scatter NULL rows across several groups. The array path (UnsafeArrayWriter) is unaffected: it writes a null element through setNull8Bytes, which zeroes the element's offset-and-size slot (size 0).

Does this PR introduce any user-facing change?

No change in a default configuration -- the nanosecond timestamp types are behind the spark.sql.timestampNanosTypes.enabled preview flag (off by default). With the flag enabled, null nanosecond keys now group and compare canonically.

How was this patch tested?

New UnsafeRowConverterSuite test asserting that two null nanosecond projections built after different non-null values are byte-identical, in both the interpreted and the codegen paths. It fails without this change and passes with it.

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

stevomitric and others added 2 commits September 11, 2026 16:00
…second timestamp to UnsafeRow

### What changes were proposed in this pull request?

`UnsafeWriter.write(int, TimestampNanosVal)` stores a nanosecond timestamp as a 16-byte
variable-length payload (modeled on `CalendarInterval`) and reserves that space even for a
null value so the slot can be updated in place later. On the null branch it set the null bit
but left the reserved 16 bytes untouched. The writer's buffer is reused across rows, so a null
nanosecond value inherited whatever bytes the previously written row left in that slot.

Two logically-equal null rows could therefore produce different `UnsafeRow` byte contents,
breaking the invariant that equal rows encode identically -- the invariant that `UnsafeRow`
hashing and equality rely on. This is the root cause of a nullable nanosecond-timestamp
GROUP BY / join key splitting its NULLs across multiple groups.

The fix zeroes the reserved payload on the null branch, mirroring the in-place update path
(`UnsafeRow.setTimestampNTZNanos` / `setTimestampLTZNanos`), which already calls
`TimestampNanosRowValues.zeroPayload`.

### Why are the changes needed?

Correctness: a null nanosecond timestamp must encode canonically so that null grouping/join
keys compare and hash identically. Without it, `GROUP BY` (and any key-based operator) over a
nullable `TIMESTAMP_NTZ(p)` / `TIMESTAMP_LTZ(p)` column can silently scatter NULL rows across
several groups. The array path (`UnsafeArrayWriter`) is unaffected: it writes a null element
through `setNull8Bytes`, which zeroes the element's offset-and-size slot (size 0).

### Does this PR introduce any user-facing change?

No change in a default configuration -- the nanosecond timestamp types are behind the
`spark.sql.timestampNanosTypes.enabled` preview flag (off by default). With the flag enabled,
null nanosecond keys now group and compare canonically.

### How was this patch tested?

New `UnsafeRowConverterSuite` test asserting that two null nanosecond projections built after
different non-null values are byte-identical, in both the interpreted and the codegen paths.
It fails without this change and passes with it.

Co-authored-by: Isaac <no-reply@databricks.com>
…nce in the comment

The added comment referenced a nonexistent `UnsafeRow#setTimestampNanos`; the method it
mirrors is the private `UnsafeRow#setTimestampNanosPayload`, which zeroes the payload on the
null-update path. Correct the cross-reference so it is greppable.

Co-authored-by: Isaac <no-reply@databricks.com>
@stevomitric

Copy link
Copy Markdown
Contributor Author

cc @uros-b PTAL.

@uros-b

uros-b commented Sep 12, 2026

Copy link
Copy Markdown
Member

LGTM, thank you @stevomitric!

@uros-b

uros-b commented Sep 12, 2026

Copy link
Copy Markdown
Member

Please fix the Jira ID in the PR title

@stevomitric stevomitric changed the title [SPARK-56822][SQL] Zero the reserved payload when writing a null nanosecond timestamp to UnsafeRow [SPARK-59442][SQL] Zero the reserved payload when writing a null nanosecond timestamp to UnsafeRow Sep 13, 2026
@stevomitric

Copy link
Copy Markdown
Contributor Author

Please fix the Jira ID in the PR title

Should point to the right ticket now. Thanks for the review @uros-b!

@uros-b uros-b closed this in 07d9266 Sep 14, 2026
uros-b pushed a commit that referenced this pull request Sep 14, 2026
…second timestamp to UnsafeRow

### What changes were proposed in this pull request?

`UnsafeWriter.write(int, TimestampNanosVal)` stores a nanosecond timestamp as a 16-byte variable-length payload (modeled on `CalendarInterval`) and reserves that space even for a null value so the slot can be updated in place later. On the null branch it set the null bit but left the reserved 16 bytes untouched. The writer's buffer is reused across rows, so a null nanosecond value inherited whatever bytes the previously written row left in that slot.

Two logically-equal null rows could therefore produce different `UnsafeRow` byte contents, breaking the invariant that equal rows encode identically -- the invariant that `UnsafeRow` hashing and equality rely on. This is the root cause of a nullable nanosecond-timestamp GROUP BY / join key splitting its NULLs across multiple groups.

The fix zeroes the reserved payload on the null branch, mirroring the in-place update path (`UnsafeRow.setTimestampNTZNanos` / `setTimestampLTZNanos`), which already calls `TimestampNanosRowValues.zeroPayload`.

### Why are the changes needed?

Correctness: a null nanosecond timestamp must encode canonically so that null grouping/join keys compare and hash identically. Without it, `GROUP BY` (and any key-based operator) over a nullable `TIMESTAMP_NTZ(p)` / `TIMESTAMP_LTZ(p)` column can silently scatter NULL rows across several groups. The array path (`UnsafeArrayWriter`) is unaffected: it writes a null element through `setNull8Bytes`, which zeroes the element's offset-and-size slot (size 0).

### Does this PR introduce any user-facing change?

No change in a default configuration -- the nanosecond timestamp types are behind the `spark.sql.timestampNanosTypes.enabled` preview flag (off by default). With the flag enabled, null nanosecond keys now group and compare canonically.

### How was this patch tested?

New `UnsafeRowConverterSuite` test asserting that two null nanosecond projections built after different non-null values are byte-identical, in both the interpreted and the codegen paths. It fails without this change and passes with it.

### What changes were proposed in this pull request?

### Why are the changes needed?

### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

### Was this patch authored or co-authored using generative AI tooling?

Closes #58746 from stevomitric/stevomitric/nanos-groupby-null-key.

Authored-by: Stevo Mitric <stevomitric2000@gmail.com>
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
(cherry picked from commit 07d9266)
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
@uros-b

uros-b commented Sep 14, 2026

Copy link
Copy Markdown
Member

Merge Summary:

Posted by merge_spark_pr.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants